Skip to content

test: allow scheduling headroom in timeout regressions - #199

Merged
steipete merged 1 commit into
mainfrom
test/timeout-regressions-20260907
Sep 7, 2026
Merged

test: allow scheduling headroom in timeout regressions#199
steipete merged 1 commit into
mainfrom
test/timeout-regressions-20260907

Conversation

@steipete

@steipete steipete commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Real subprocess timeout tests intermittently failed on busy hosts because the 1.5-second return assertion included process startup and scheduling delays. The prior sweep reproduced this on unchanged main (1.65–1.89 seconds).

Keep explicit 5-second return assertions, timeout exit codes, and timeout error checks. Give the test runner additional setup/cleanup headroom. Production timeouts are unchanged.

Validation: typecheck and lint passed; the full suite passed (924 tests, 2 platform skips), and the final focused run passed (23 tests, 2 platform skips). The built CLI completed init/map/status on a synthetic Node/Rust repository. Codex autoreview of the working change was clean at P2 after preserving explicit return-time assertions.

Local package smoke also exposed a separate pre-existing compatibility issue: a globally installed pnpm 12 rejects the prepack script's -s alias before honoring the pinned package manager. That repair belongs to the final tooling/notes PR. This PR contains tests only; the final notes PR will consolidate the maintenance entry.

Targeted runtime proof

Actual timeout proof from an isolated checkout built at dd75865cc35593816832814bf11aafbe6654d00a, on macOS arm64 / Node 24.20.0:

Scenario Configured deadline Result Elapsed Readiness confirmed
inherited stdio descendant 500 ms exit 124 1006 ms yes
git push 80 ms exit 1 987 ms yes
gh pr create 500 ms exit 7 1192 ms yes

The inherited-stdio probe ran a real parent and descendant through the built dist/exec.js. The publishing probes ran the built CLI:

node dist/cli.js open-pr --root <fixture> --patch pat_timeout_proof --base main --branch clawpatch/pat_timeout_proof --json --quiet

A local bare Git remote and hanging executables exercised git push and gh pr create; no external push or PR creation occurred. Each process wrote a readiness marker before its timeout. All three errors contained command timed out after <configured deadline>ms. The 500 ms probes allow process startup before testing the hang; the regression tests retain their 50/80 ms deadlines and 5-second return bounds.

The exact-head Linux/Windows CI remains green: https://github.com/openclaw/clawpatch/actions/runs/34112758456. This supplies the requested inspectable timeout behavior in addition to the already-passing test suite and clean P2 branch autoreview.

@clawsweeper

clawsweeper Bot commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@steipete

steipete commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Prepared head: dd75865. Both the complete suite (924 passed, 2 platform skips) and final focused timeout run (23 passed, 2 platform skips) passed locally; typecheck/lint passed. Built CLI init/map/status succeeded on a synthetic Node/Rust repository. The configured 50/80ms command timeouts still produce timeout failures and explicit bounded-return assertions remain.

CI is green on this head: https://github.com/openclaw/clawpatch/actions/runs/34112758456 (Linux full suite/package smoke and Windows exec tests). CodeQL and secret scans passed. Codex branch autoreview against origin/main is scoped-clean at P2 after addressing its initial concern about preserving explicit return bounds.

Ready for orchestrator squash; not merged in this preparation phase. The final notes PR will carry the maintenance changelog entry.

@clawsweeper

clawsweeper Bot commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 7, 2026, 7:32 AM ET / 11:32 UTC.

ClawSweeper review

What this changes

The PR gives three subprocess timeout tests more scheduling headroom while retaining explicit return-time bounds and timeout error checks.

Merge readiness

✅ Ready for maintainer review

This remains a useful, bounded test reliability change absent from current main. The new runtime evidence resolves the previous proof blocker, and no actionable patch defects remain.

Priority: P3
Reviewed head: dd75865cc35593816832814bf11aafbe6654d00a

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, well-supported test adjustment with relevant runtime evidence and no remaining actionable findings.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): The supplied pinned-head runtime results exercise the built command runner and publishing CLI with real hanging subprocesses, readiness markers, timeout diagnostics, and bounded elapsed times for all three affected scenarios. They resolve the previous proof request; CI and unit results are supplemental.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The supplied pinned-head runtime results exercise the built command runner and publishing CLI with real hanging subprocesses, readiness markers, timeout diagnostics, and bounded elapsed times for all three affected scenarios. They resolve the previous proof request; CI and unit results are supplemental.
Evidence reviewed 8 items Repository policy and scope: Read the complete root AGENTS.md and verified the origin repository. No nested AGENTS.md or matching maintainer notes were found. Applied the focused-test and generated-output guidance.
Verified introduced change: The pinned base-to-head diff changes only two test files: 9 additions and 6 deletions. The inherited-stdio test retains its 50ms command deadline, exit 124 assertion, and timeout message; publishing tests retain their 80ms deadlines and distinct error checks. All three retain explicit 5-second return bounds.
Production timeout boundary: Publishing passes configured deadlines to the shared command runner for git push and gh pr create. The runner terminates timed-out subprocesses, destroys output streams after cleanup, and returns exit 124. None of this production behavior is changed.
Findings None None.
Security None None.

How this fits together

Clawpatch runs subprocesses for validation and publishing pull requests. These tests check that hanging commands and inherited output pipes return bounded timeout failures to the CLI.

flowchart LR
  A[Timeout regression tests] --> B[Command runner]
  A --> C[Pull request publishing]
  C --> B
  B --> D[Hanging subprocesses]
  D --> E[Deadline and process cleanup]
  E --> F[Timeout errors and bounded return]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

None.

Technical review

Best possible solution:

Keep realistic scheduling allowances in subprocess tests while preserving short command deadlines, explicit return bounds, and timeout failure assertions.

Do we have a high-confidence way to reproduce the issue?

Not applicable to a production bug: this PR adjusts test timing tolerance. The contributor reports unchanged-main failures at 1.65–1.89 seconds; this review did not independently reproduce host scheduling delays.

Is this the best way to solve the issue?

Yes. The narrow test-only adjustment preserves meaningful hang detection and timeout diagnostics without changing operator deadlines or adding configuration.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 9f71d92f0069.

Labels

Label changes:

  • add P3: This is a focused test reliability improvement with no production behavior change.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The supplied pinned-head runtime results exercise the built command runner and publishing CLI with real hanging subprocesses, readiness markers, timeout diagnostics, and bounded elapsed times for all three affected scenarios. They resolve the previous proof request; CI and unit results are supplemental.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The supplied pinned-head runtime results exercise the built command runner and publishing CLI with real hanging subprocesses, readiness markers, timeout diagnostics, and bounded elapsed times for all three affected scenarios. They resolve the previous proof request; CI and unit results are supplemental.

Label justifications:

  • P3: This is a focused test reliability improvement with no production behavior change.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The supplied pinned-head runtime results exercise the built command runner and publishing CLI with real hanging subprocesses, readiness markers, timeout diagnostics, and bounded elapsed times for all three affected scenarios. They resolve the previous proof request; CI and unit results are supplemental.
  • proof: sufficient: Contributor real behavior proof is sufficient. The supplied pinned-head runtime results exercise the built command runner and publishing CLI with real hanging subprocesses, readiness markers, timeout diagnostics, and bounded elapsed times for all three affected scenarios. They resolve the previous proof request; CI and unit results are supplemental.

Evidence

What I checked:

  • Repository policy and scope: Read the complete root AGENTS.md and verified the origin repository. No nested AGENTS.md or matching maintainer notes were found. Applied the focused-test and generated-output guidance. (AGENTS.md:1, dd75865cc355)
  • Verified introduced change: The pinned base-to-head diff changes only two test files: 9 additions and 6 deletions. The inherited-stdio test retains its 50ms command deadline, exit 124 assertion, and timeout message; publishing tests retain their 80ms deadlines and distinct error checks. All three retain explicit 5-second return bounds. (src/exec.test.ts:142, dd75865cc355)
  • Production timeout boundary: Publishing passes configured deadlines to the shared command runner for git push and gh pr create. The runner terminates timed-out subprocesses, destroys output streams after cleanup, and returns exit 124. None of this production behavior is changed. (src/open-pr.ts:149, 9f71d92f0069)
  • Current-main and release necessity: The current-main test still has the 4-second publishing test deadline, and v0.7.3 still has the 1.5-second inherited-stdio assertions. The requested scheduling allowance is not already implemented in the inspected default branch or latest release. (src/open-pr.test.ts:11, 9f71d92f0069)
  • After-change runtime proof: The complete supplied body and test: allow scheduling headroom in timeout regressions #199 (comment) report execution at the pinned head on macOS arm64 with Node 24.20.0. Real subprocess probes through built dist/exec.js and the built open-pr CLI returned timeout results in 1006ms, 987ms, and 1192ms for inherited stdio, git push, and gh pr create respectively. Readiness markers confirmed that the hanging processes started; all results contained the configured timeout message. This directly addresses the previous requested runtime transcript. (dd75865cc355)
  • Supplemental validation: The supplied discussion reports 924 passing tests with two platform skips, a final focused run of 23 passing tests with two skips, and successful typecheck/lint. Captured check results show successful Linux tests and Windows exec tests. No tests or builds were executed during this read-only review. (dd75865cc355)

Likely related people:

  • Sebastien Tardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-07T10:43:38.738Z sha dd75865 :: needs real behavior proof before merge. :: none

@steipete steipete mentioned this pull request Sep 7, 2026
Merged
@steipete

steipete commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Actual timeout proof from an isolated checkout built at dd75865cc35593816832814bf11aafbe6654d00a, on macOS arm64 / Node 24.20.0:

Scenario Configured deadline Result Elapsed Readiness confirmed
inherited stdio descendant 500 ms exit 124 1006 ms yes
git push 80 ms exit 1 987 ms yes
gh pr create 500 ms exit 7 1192 ms yes

The inherited-stdio probe ran a real parent and descendant through the built dist/exec.js. The publishing probes ran the built CLI:

node dist/cli.js open-pr --root <fixture> --patch pat_timeout_proof --base main --branch clawpatch/pat_timeout_proof --json --quiet

A local bare Git remote and hanging executables exercised git push and gh pr create; no external push or PR creation occurred. Each process wrote a readiness marker before its timeout. All three errors contained command timed out after <configured deadline>ms. The 500 ms probes allow process startup before testing the hang; the regression tests retain their 50/80 ms deadlines and 5-second return bounds.

The exact-head Linux/Windows CI remains green: https://github.com/openclaw/clawpatch/actions/runs/34112758456. This supplies the requested inspectable timeout behavior in addition to the already-passing test suite and clean P2 branch autoreview.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 7, 2026
@steipete steipete mentioned this pull request Sep 7, 2026
Merged
@steipete
steipete merged commit dcc17c6 into main Sep 7, 2026
13 checks passed
steipete added a commit that referenced this pull request Sep 7, 2026
@steipete
chore: prepare 0.8.0 notes and refresh pnpm setup
60ccb46 
Use the portable prepack build invocation, update pnpm/action-setup to 6.1.0, and consolidate the release notes for #199 and #200.

Retain pnpm 11.25.0 and the existing single-document lockfile.
@steipete
steipete deleted the test/timeout-regressions-20260907 branch September 7, 2026 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete